-
Notifications
You must be signed in to change notification settings - Fork 668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PR to merge ARM SVE branch #315
Conversation
I've created release package for testing this w/o the 'maintainer mode' requirements. See there: https://github.com/rdolbeau/fftw3/releases/tag/sve-test-release-001 |
@stevengj @matteo-frigo Can you comment on this? In particular the "used fixed-width implementation and enable all those of equal-or-narrower to the hardware width". It's a bit hackish but it's the best we have at this time. RISC-V is using the same principle for now. |
I would like to encourage the FFTW folks to consider this PR so it can be included (after some cleanup) into the next FFTW release. At this stage, my main interest is only about ARM SVE.
Though SVE vector length can scale from 128 to 2048 bits (via 128 bits increments), only 3 sizes are available today (128, 256 and 512. Fujitsu A64fx does not support 384 bits). So even if vector length agnostic FFTW might be achievable, I do not see this as mandatory because of the currently limited available vector lengths and the fact VLA code is (generally) slightly slower than vector length specific code. If needed, I can provide an option to set a maximum supported vector length in order to reduce the size of the FFTW library. Bottom line, I approve the approach of the PR and hope it can make its way upstream. |
Also, in its current incarnation SVE doesn't allow for non-power-of-2 multiple of 128 bits anymore, only power-of-2 multiple of 128 bit are. This is visible for instance in DDI0487J for the register ZCR_EL1: So this implementation should cover all cases, and I agree with you that 1024 and 2048 bits are currently only for emulators like QEmu and could (should?) be optional. 512 and 256 could also be optional for site-specific deployment. |
Done some minor clean-ups, minor improvement, and rebase to current HEAD. @ggouaillardet if you have the time to confirm this is still OK for you |
I've created a new package with pre-generated files, so maintainer mode is not required to test this on Arm+SVE, see https://github.com/rdolbeau/fftw3/releases/tag/sve-test-release-002 |
Thanks @rdolbeau and my apologies for the late reply. I noted |
Probably with all compilers that do not enable SVE by default (all of them, probably!), currently configure doesn't enable it explicitly so it has to be done in {C/CXX/F}FLAGS.
Good idea. I'll try that and add it to the PR ASAP. |
I can see performance benefits by extending to 256 bits and above. Do you happen to have performance data for the sve version comparing to the SIMD NEON version? |
Here is some performance number I collected a while ago for Feel free to suggest other/better benchmarks if appropriate. |
Hello, I ran few experiments using the included benchmark
Where Baseline stands for the current release 3.3.10 built with We can observe significant gain on all SVE machines (A64FX, Neoverse V1 and V2), as well as no degradation on NEON only machine (Neoverse N1). Best regards. |
These results are very exciting. FFTW performance is extremely important to users of NVIDIA Grace (Neoverse V2), which is now the dominant architecture on the Green500. A great many people would be glad to see this commit in the next FFTW release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of nits. Feel free to address them or not. The structure looks good overall, and thanks for the benchmarking results as well. They speak for themsleves
@@ -0,0 +1,84 @@ | |||
#include <stdio.h> | |||
#include <string.h> | |||
#include <stdlib.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You gate the include later in this file, so remove this
unsigned int size = rp2(osize); | ||
if (osize != size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really important, but checking for a power of two can be done with:
if (!(osize & (osize - 1))
just a nice tidbit from Hacker's Delight
simd-support/simd-maskedsve.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not to be included directly (accessed through, e.g. simd-maskedsve1024.h), so should it be named something else, e.g. simd-maskedsve.h.template?
and add simd-support/{generate_vtw.sh,vtw.h} into the dist tarball
ignore all files automatically generated for SVE support
ADD/SUB/MUL are three-addresses in SVE, but the masked form is only two-adresses. And there's a lot of reuse in FFTW3 (and complex arithmetic). But ACLE/SVE (i.e. intrinsics) don't have the non-masked form :-( So used inline ASM for force the non-masked version to be used. Masked-out lanes should be mostly zero, and are never stored anyway, so computing on them should be fine. This one will be reversed if it's not a performance win.
…mpiler/hardware dependent, and more tests are needed before settling on some defaults.
When configure'd with --enable-sve, try to build a sample SVE program and abort on failure, otherwise configure successes but make will fail.
I've created a new package with pre-generated files, so maintainer mode is not required to test this on Arm+SVE, see https://github.com/rdolbeau/fftw3/releases/tag/sve-test-release-003 |
@stevengj @matteo-frigo any comment on this? Is this OK to think about merging ? |
It's been a while, so time to try and get some attention to this again... Can this be merged, and another FFTW version be released that includes these changes? |
@stevengj @matteo-frigo any comment on this? Is this OK to merge the branch and gain mor eepxosure and feedback ? At this stage, lots of positive comments, and the code has been extensively tested on all available SVE width: 128 (on Nvidia Grace / Neoverse V2), 256 (AWS Graviton 3 / Neoverse V1), 512 (Fujitsu A64FX). |
The performance improvements are shown here for sizes from 512 to 36864 that can be decomposed into factors of 2, 3, 5, and 7. FFTW_MEASURE flag is used to generate the plans and MFLOPS is defined as (5N log_2 N)/t, where t is the time and N is the size of transform. Best regards. |
I merged this pull request but I'd like to change a few things.
Is it good enough? |
@rdolbeau @tetsuzo-usui I have pushed some cleanups onto master, can you check that I haven't broken anything? |
This was expanded to support RISC-V V in addition to Arm SVE. They both use the same "one set of codelets by sizes + masking" idea, up to the maximum architecture size.
No, it's just auto-generated from a script because that was easy to do. Macros are fine if they produce the same arrays.
There's SVE (presumably 128 bits), so if you can compile/test FFTW3 it should be OK hardware-wise (only regular SVE is needed, which is all that is supported on the A64FX & Neoverse V1, other CPUs also have SVE2 but that doesn't bring much to the table for numerical codes). You probably want a recent version of GCC or LLVM to check the performance. There's a known "issue" with the way the code is written: as the same source is used for all sizes and use masking, only masked instructions are used. But in SVE, some masked instructions (including Other than that, server-class, you can test in the cloud(s): AWS Graviton 3 (Neoverse V1), AWS Graviton 4 (Neoverse V2), Microsoft Cobalt 100 (Neoverse N2) should all be good. NVidia Grace (Neoverse V2) hardware can be bought for on-premise use and is also good. Ampere Altra (Neoverse N1) is NOT - no SVE there. Anything else with FEAT_SVE should be good as well, but as far as I know no Apple hardware support SVE. Also, of interest to anyone looking at NEON vs. SVE: this hardware has FEAT_FCMA, along all other hardware I know of with FEAT_SVE. That means it can use the complex FP instructions in NEON (fcmla, fcadd), but the current code in NEON doesn't use them as FEAT_FCMA is a recent addition to NEON. I did a quick test on a Neoverse V2, and adding FEAT_FCMA support in NEON does help the NEON code. However, that would require two sets of NEON codelets to have a "universal" library, which may not be desirable. As SVE offers access to those instructions anyway, I didn't make a clean version of the patch as I don't think it's needed, but if someone has a a different opinion I can dig it up. |
Thanks. I have now mostly unified the SVE and AVX* macros for the twiddle factor storage, so things should be simpler next time. The code works on my phone (128-bit sve). I'll try the cloud next. |
@rdolbeau I started a m8g.large aws instance. It has SVE but only 128-bit. Is that expected? Should I read your message as implying that Graviton 3 has 256-bit SVE and Graviton 4 has 128-bit SVE? |
Yes and yes. m8g is Graviton 4 (https://aws.amazon.com/ec2/instance-types/m8g/), using the Neoverse V2 core, which has 128 bits SVE (and four pipelines of it). Graviton 3 uses the Neoverse V1 core which has 256 bits SVE (two pipelines, which are the same as the four NEON pipelines but aggregated in pairs). Those would be e.g. c7g or m7g. For both systems, the theoretical Flops throughput is the same in NEON and SVE (16 flops/cycles incl. FMA),, and only twice the scalar throughput (8 flops per cycle incl. FMA). A64FX is the one where SVE give the most theoretical benefits (only two pipeleines like on Intel systems, 512 bits max, so 32 flops/cycle incl. FMA in SVE, 8 flops/cycle incl. FMA in NEON, and 4 flops/cycle incl. FMA in scalar). fcmla/fcadd are a significant help to performance in SVE vs. the current NEON code, as mentioned above. |
@rdolbeau @tetsuzo-usui What's the right way to handle the cycle counter these days on arm64? It looks like one has to configure with --enable-armv8-cntvct-el0 Is this supported on all armv8 so that we can enable it automatically? Is there some intrinsic nowadays that does the job? |
I think it is architecturally defined and therefore should be OK on all Arm v8 (and v9, which is just a renaming of v8), so could be set as default on v8. I don't know of an intrinsic to access the counter directly, but it might exist in some compilers? IIRC it's just a line ot two of ASM in the current code. |
The latest FFTW in the current master branch has been tested for numerical results and performance on Grace, Graviton 3 and A64FX, and all are fine. About 1,000 randomly selected tests were run with "make check", and all passed in double and single precision on all machines. An excerpt of the output log is shown below:
The difference in average performance of sizes from 512 to 36864 compared to Dolbeau's PR performance is about 1% on each platform, which is not a large difference beyond measurement error. |
This is primarily for discussion ad probably need some cleaning up.
The current scheme using SVE (and RISC-V V) doesn't leverage the scalability; instead, it produces sets of codelets for all possible power-of-2 sizes, using masking. Only sets of a size <= to the hardware implementation width are enabled. See here.
It also auto-generate the simd-support/vtw.h file that defines the various VTW macro. This is useful as while SVE is limited to 2048 bits wide vector, RISC-V V is more or less unbounded and there's a least a 16384 bits wide implementation being developed.
So using SVE (or V) creates a fairly large, though versatile, library.
I has been tested in QEmu; using the Arm instruction Emulator; and on real hardware (Fujitsu A64FX, AWS Graviton 3).